fix: Add SELinux support and dynamic UID/GID matching for Docker #398
Conversation
Fixes Docker volume permission issues on Fedora/RHEL systems with SELinux enforcing. Addresses two root causes: 1. SELinux blocking container access to host volumes (fixed with :z flag) 2. UID/GID mismatch between host user (1000) and container user (1001) Changes: - Add SELinux :z flags to all bind mount declarations in docker-compose files - Implement dynamic UID/GID matching via build args (USER_ID/GROUP_ID) - Add safety guard to prevent creating root user (UID/GID 0) - Remove conflicting node user (UID 1000) in base image before creating automaker user - Automatically pass host UID/GID from launcher script via process.getuid()/getgid() - Update documentation with SELinux examples in README and docker-isolation.md The :z flag tells Docker to relabel volumes for SELinux, required on Fedora/RHEL but safely ignored on other systems. UID/GID matching ensures container processes can read/write host files regardless of host user UID. Tested on Fedora 43 with SELinux enforcing and verified backwards compatibility with non-SELinux systems.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaces fixed non-root UID/GID with build-time ARGs (USER_ID, GROUP_ID), adds validation and conflict cleanup in Dockerfiles, propagates host UID/GID from the launcher into Docker build/run, and updates compose mounts and docs to use SELinux-friendly Changes
Sequence Diagram(s)sequenceDiagram
actor Developer
participant Launcher as launcher-utils.mjs
participant Docker as Docker daemon
participant Build as Dockerfile build
participant Container
Developer->>Launcher: run dev command
Launcher->>Launcher: getHostUserCredentials() -> USER_ID,GROUP_ID
Launcher->>Docker: docker build (args: USER_ID, GROUP_ID)
Docker->>Build: pass build args
Build->>Build: validate IDs, remove conflicts, create group/user, set home perms
Launcher->>Docker: docker run (env: USER_ID, GROUP_ID, mount :z)
Docker->>Container: start container with matching UID/GID and relabeled volumes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @igouss, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances Docker container compatibility and user experience, particularly for users on Fedora/RHEL systems with SELinux enabled. It addresses two primary pain points: SELinux blocking container access to host volumes and UID/GID mismatches between host and container users. By introducing dynamic UID/GID matching and integrating SELinux Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces excellent improvements for Docker compatibility, specifically adding SELinux support via the :z flag and implementing dynamic UID/GID matching to resolve volume permission issues. The changes are well-implemented across the Dockerfiles, docker-compose files, and launcher scripts. The documentation has also been updated clearly to reflect these new features. I've identified a couple of areas for potential improvement regarding code duplication and startup performance, which I've detailed in the comments.
docker-compose.dev.yml
Outdated
| chown -R automaker:automaker /app/node_modules 2>/dev/null || true | ||
| # Ensure automaker user can write to /app for npm operations | ||
| # This is safe since /app is volume-mounted from host | ||
| chown automaker:automaker /app 2>/dev/null || true |
There was a problem hiding this comment.
Running chown -R on node_modules at every container start can significantly slow down startup time, especially for large projects. To optimize this, you could make these chown commands conditional, so they only run when the ownership is incorrect. This would improve the developer experience by speeding up subsequent container starts.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/launcher-utils.mjs (1)
1019-1090: Add USER_ID and GROUP_ID environment variables to production Docker launch.The
launchDockerContainersfunction doesn't passUSER_IDandGROUP_IDenvironment variables, while bothlaunchDockerDevContainersandlaunchDockerDevServerContainerdo. Sincedocker-compose.ymlrequires these variables (lines 32-33:USER_ID: ${USER_ID:-1000}andGROUP_ID: ${GROUP_ID:-1000}), the production launch should also pass them to avoid permission issues with volume mounts.Update the
envobject to include:USER_ID: process.getuid?.()?.toString() || '1000', GROUP_ID: process.getgid?.()?.toString() || '1000',
🧹 Nitpick comments (1)
scripts/launcher-utils.mjs (1)
937-939: Consistent UID/GID handling, but consider extracting the common pattern.This duplicates the same environment variable logic from
launchDockerDevContainers. Consider extracting this into a helper function to avoid repetition.♻️ Suggested refactor to reduce duplication
Extract the common environment setup:
+/** + * Get host user credentials for Docker volume permission matching + * @returns {{USER_ID: string, GROUP_ID: string}} + */ +function getHostUserCredentials() { + return { + USER_ID: process.getuid?.()?.toString() || '1000', + GROUP_ID: process.getgid?.()?.toString() || '1000', + }; +}Then use it in both functions:
env: { ...process.env, - // Pass host UID/GID to avoid permission issues with volume mounts - USER_ID: process.getuid?.()?.toString() || '1000', - GROUP_ID: process.getgid?.()?.toString() || '1000', + ...getHostUserCredentials(), },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
DockerfileDockerfile.devREADME.mddocker-compose.dev-server.ymldocker-compose.dev.ymldocker-compose.override.yml.exampledocker-compose.ymldocs/docker-isolation.mdscripts/launcher-utils.mjs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-07T15:21:11.144Z
Learnt from: webdevcody
Repo: AutoMaker-Org/automaker PR: 378
File: apps/server/src/lib/sdk-options.ts:141-148
Timestamp: 2026-01-07T15:21:11.144Z
Learning: AutoMaker intentionally uses `permissionMode: 'bypassPermissions'` with `allowDangerouslySkipPermissions: true` in base SDK options for fully autonomous operation. This is an accepted architectural decision, with Docker containerization as the primary security boundary.
Applied to files:
docker-compose.dev.yml
🔇 Additional comments (20)
README.md (2)
243-248: LGTM! Clear SELinux guidance.The documentation correctly explains the SELinux
:zflag requirement for Fedora/RHEL systems and clarifies that it's safely ignored on other systems. The example is clear and practical.
299-311: LGTM! Comprehensive example with consistent SELinux support.The complete docker-compose override example consistently applies the
:zflag to all volume mounts and reinforces the SELinux requirement. This provides a clear, copy-pasteable reference for users.Dockerfile (4)
81-89: LGTM! Critical safety guard implemented correctly.The root user prevention check is essential for security and correctly prevents creating a container user with UID or GID 0. The
set -eensures the build fails immediately if root is attempted.
90-96: LGTM! Robust conflict resolution.The logic correctly handles the case where the base image's node user (UID 1000) conflicts with the desired automaker user ID. Using
getentto check for existing users/groups and|| trueto handle deletion failures is appropriate.
97-99: LGTM! Correct user creation with dynamic IDs.The user and group creation logic correctly applies the provided USER_ID and GROUP_ID build arguments. The useradd flags are appropriate for setting up a functional non-root user.
100-103: LGTM! Proper home directory setup with secure permissions.The directory creation and ownership setup is correct. The explicit
chmod 700on.cursorensures secure, owner-only access to sensitive authentication data.docker-compose.override.yml.example (2)
5-7: LGTM! Correct SELinux-aware volume mount.The
:rw,zflag combination correctly enables both read-write access and SELinux relabeling. The comment clearly explains the SELinux requirement.
11-19: LGTM! Consistent SELinux support for CLI authentication.The commented-out examples correctly demonstrate the
:zflag on CLI credential mounts. Not using:rois appropriate since CLI tools need write access to these directories.docs/docker-isolation.md (2)
49-63: LGTM! Comprehensive SELinux guidance with security best practices.The documentation correctly demonstrates both
:ro,z(read-only + SELinux) and:z(read-write + SELinux) patterns. Recommending:rowhen possible is a good security practice.
113-118: LGTM! Correct CLI authentication mount guidance.The volume mounts correctly apply the
:zflag for SELinux compatibility. Using read-write (no:ro) is appropriate since CLI tools need to write authentication data.docker-compose.yml (1)
31-33: Build args correctly configured for dynamic UID/GID matching.The build arguments properly use environment variable fallback with sensible defaults (1000). The launcher script correctly passes these values via
process.getuid()andprocess.getgid()in both development container modes, avoiding permission issues with volume mounts.scripts/launcher-utils.mjs (1)
872-874: Good approach for dynamic UID/GID matching with appropriate fallback.The optional chaining correctly handles Windows where
process.getuid()andprocess.getgid()are undefined. The fallback to '1000' is reasonable since Docker Desktop on Windows runs containers in a Linux VM.docker-compose.dev.yml (3)
21-23: LGTM! Proper build-time UID/GID configuration.The build args correctly propagate host user credentials to the container build, enabling dynamic user creation that matches the host UID/GID. This prevents permission issues with volume mounts.
Also applies to: 104-106
54-55: Correct SELinux relabeling flag for bind mounts.The
:zflag properly enables SELinux relabeling for per-container access on Fedora/RHEL systems. This is a no-op on non-SELinux systems, ensuring backwards compatibility.Also applies to: 119-120
76-81: Security consideration: chowning bind-mounted directory.The entrypoint changes ownership of
/app(line 81), which is a bind mount of the host's project directory. While this resolves permission issues for npm operations inside the container, it modifies ownership of the host directory. If the container is compromised, this could allow unauthorized writes to the host filesystem within the mounted path.Consider these alternatives:
- Preferred: Create a subdirectory for npm cache/temp files that doesn't require chowning the entire
/app- Alternative: Use the
--chownflag in the volume mount (requires Docker 17.09+):.:/app:z,uid=${USER_ID},gid=${GROUP_ID}- Document: If the current approach is intentional, add a security note in the documentation about the trust boundary and Docker containerization as the security layer (consistent with the learning about
permissionMode: 'bypassPermissions')Based on learnings, this aligns with AutoMaker's architectural decision to rely on Docker containerization as the primary security boundary.
⛔ Skipped due to learnings
Learnt from: webdevcody Repo: AutoMaker-Org/automaker PR: 378 File: apps/server/src/lib/sdk-options.ts:141-148 Timestamp: 2026-01-07T15:21:11.144Z Learning: AutoMaker intentionally uses `permissionMode: 'bypassPermissions'` with `allowDangerouslySkipPermissions: true` in base SDK options for fully autonomous operation. This is an accepted architectural decision, with Docker containerization as the primary security boundary.docker-compose.dev-server.yml (2)
20-22: Consistent with dev.yml configuration.The build args and SELinux
:zflag are correctly configured, matching the pattern indocker-compose.dev.yml.Also applies to: 53-54
75-77: Better security posture than docker-compose.dev.yml.This file only chowns
/app/node_modules(line 77) rather than the entire/appdirectory, which is safer since it limits write access to only the necessary subdirectory. Consider applying this more restrictive approach todocker-compose.dev.ymlas well (see comment on that file).Dockerfile.dev (3)
30-38: Excellent safety guard against root user creation.The validation prevents creating a container user with UID/GID 0, which would be a security risk. This ensures the container always runs with a non-root user.
46-52: Proper dynamic user creation with correct permissions.The user and group creation uses the provided build args, and home directory permissions are properly set. This enables seamless UID/GID matching between host and container.
40-45: User/group deletion with appropriate error handling.The script removes conflicting users and groups (including the base image's
nodeuser at UID 1000) before creating theautomakeruser. Theuserdel -fflag and|| truefallback correctly handle the edge case where deletion might fail or where the user owns files—the-fflag forcefully removes the user entry even if files exist, which is the intended behavior.
- Extract getHostUserCredentials() helper to reduce code duplication - Add USER_ID/GROUP_ID to production Docker launcher - Only chown /app/node_modules in dev.yml (not entire /app directory) Addresses review comments from CodeRabbit and Gemini Code Assist: - Reduces security surface by not modifying host directory ownership - Ensures production mode has same UID/GID support as dev modes - Eliminates code duplication across three Docker launcher functions Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docker-compose.dev.ymlscripts/launcher-utils.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose.dev.yml
🔇 Additional comments (4)
scripts/launcher-utils.mjs (4)
883-884: LGTM! Clean integration of host credentials.The spread operator usage correctly injects host UID/GID into the Docker environment, and the comment clearly explains the purpose.
947-948: LGTM! Consistent pattern applied.The host credentials are correctly propagated here using the same clean pattern as in the dev containers function.
1081-1082: LGTM! Production parity achieved.Host credentials are now correctly passed in the production rebuild path, achieving parity with the dev mode as mentioned in the commit message.
1099-1100: LGTM! Complete coverage in both paths.Host credentials are correctly propagated in the non-rebuild path as well, ensuring consistent UID/GID handling regardless of whether images need rebuilding.
- Fail immediately if running as root (UID/GID 0) with clear error message - Add informational logging for Windows fallback (UID/GID 1000) - Improves security and debugging UX Addresses CodeRabbit review comment about missing root validation in the helper function. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
conflicts, please review / fix if you get time. |
|
Resolved |
2 participants
Fixes Docker volume permission issues on Fedora/RHEL systems with SELinux enforcing. Addresses two root causes:
Changes:
The :z flag tells Docker to relabel volumes for SELinux, required on Fedora/RHEL but safely ignored on other systems. UID/GID matching ensures container processes can read/write host files regardless of host user UID.
Tested on Fedora 43 with SELinux enforcing and verified backwards compatibility with non-SELinux systems.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.